Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[#6326] improve(CLI): Make CLI more extendable and maintainable. #6327

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

[#6326] improve(CLI): Make CLI more extendable and maintainable. #6327

wants to merge 4 commits into from

Conversation

Abyss-lord
Copy link
Contributor

What changes were proposed in this pull request?

Make CLI more extendable and maintainable.
design doc: https://docs.google.com/document/d/1JU20XPHBMb-boOhdNmeZC9acyYupuj7EvQKfc1JaJBo/edit?usp=sharing

Why are the changes needed?

Fix: #6326

Does this PR introduce any user-facing change?

No

How was this patch tested?

local test + ut

# ======= METALAKE =======
❯ gcli metalake details -m demo_metalake -i --output table
# +---+---------------+-------------+
# |   |   METALAKE    |   COMMENT   |
# +---+---------------+-------------+
# | 1 | demo_metalake | new comment |
# +---+---------------+-------------+

❯ gcli metalake details -m demo_metalake -i
# demo_metalake, new comment

❯ gcli metalake list  -m demo_metalake -i --output table
# +---+-------------------+
# |   |     METALAKE      |
# +---+-------------------+
# | 1 | demo_metalake     |
# | 2 | test_cli_metalake |
# | 3 | tyy               |
# | 4 | demo3             |
# | 5 | demo_metalake2    |
# +---+-------------------+

❯ gcli metalake list  -m demo_metalake -i
# demo_metalake
# test_cli_metalake
# tyy
# demo3
# demo_metalake2

❯ gcli metalake details -m demo_metalake -i --output table --quiet
# no output

# ======= CATALOG =======
❯ gcli catalog list  -m demo_metalake -i --output table
# +---+-------------------+
# |   |     CATALOG       |
# +---+-------------------+
# | 1 | File              |
# | 2 | Hive_catalog      |
# | 3 | Iceberg_catalog   |
# | 4 | Mysql_catalog     |
# | 5 | Test_hive_catalog |
# +---+-------------------+

❯ gcli catalog list  -m demo_metalake -i
# File
# Hive_catalog
# Iceberg_catalog
# Mysql_catalog
# Test_hive_catalog

❯ gcli catalog details  -m demo_metalake --name Hive_catalog  -i --output table
# +---+--------------+------------+----------+-------------+
# |   |   CATALOG    |    TYPE    | PROVIDER |   COMMENT   |
# +---+--------------+------------+----------+-------------+
# | 1 | Hive_catalog | RELATIONAL | hive     | new comment |
# +---+--------------+------------+----------+-------------+

❯ gcli catalog details  -m demo_metalake --name Hive_catalog  -i
# Hive_catalog, RELATIONAL, hive, new comment

@Abyss-lord
Copy link
Contributor Author

Abyss-lord commented Jan 19, 2025 •
edited
Loading

@justinmclean @tengqm , could you please review this PR when you have time? I’d really appreciate your feedback. plz review my new design

@tengqm
Copy link
Contributor

tengqm commented Jan 19, 2025

I'll refrain from leaving comments here till I get a whole picture about where we are heading.

@shaofengshi
Copy link
Contributor

Hi @Abyss-lord, thanks for the PR, while I feel this change is a little bit heavy, which may make the CLI code harder to maintain. Regarding the feature, would you add more information in the issue description, so that we can understand the scenario? Thank you!

@Abyss-lord
Copy link
Contributor Author

Hi @Abyss-lord, thanks for the PR, while I feel this change is a little bit heavy, which may make the CLI code harder to maintain. Regarding the feature, would you add more information in the issue description, so that we can understand the scenario? Thank you!

@shaofengshi As the CLI continues to evolve, we face growing challenges with code maintainability. There are two main issues we need to address:

  1. Parameter Management Challenge:Currently, commands require numerous individual arguments. Adding a new option requires modifications across all commands.This approach doesn't scale well as we add more functionality
  2. Inconsistent Output Handling:Output methods are fragmented between System.out/err and OutputFormat#output and Lack of standardized output handling makes the interface inconsistent

To address these issues, I propose the following solutions:
For the first question, I propose grouping related options into logical parameter clusters according to action not entity. This approach will improve code organization and maintainability. The proposed structure is illustrated in the following class diagram...

image

When the refactoring is complete the command just needs to pass the configuration + FullName, e.g.
newDeleteTable(url, ignore, force, metalake, catalog, schema, table) -> newDeleteTable(DeleteConfig config, FullName fullname)

For the second problem, I've redesigned the OutputFormat interface to improve extensibility. I refactor the OutputFormat to increase its extensibility, but also leave room for extension, such as sorting, limit and other functions.

image

Now all of the CLI's information can be displayed using a single output method.
image

Although this is a lot of code changes, IMHO I think it will be a big benefit for future client development.

design doc: https://docs.google.com/document/d/1JU20XPHBMb-boOhdNmeZC9acyYupuj7EvQKfc1JaJBo/edit?usp=sharing

@justinmclean
Copy link
Member

justinmclean commented Jan 22, 2025 •
edited
Loading

First off, I would split this into two PRs, one for the "config" and one for the output if possible. There are a few variable names / classes that need better names, but overall, I think this is a little too complex. There is no real need for xxxConfig classes right now and you have also moved a little too much into them. Each class should have a single responsibility and only contain related data. As suggested, I think at this point a command context would be a better way to go, along with a thin wrapper around the output calls. Take a look at #6343 and see what you think.

@justinmclean
Copy link
Member

justinmclean commented Jan 22, 2025 •
edited
Loading

It would be possible to extend PR #6343 to include full names in the method signatures. However, care must be taken to avoid making the implementation overly abstract. Striking the right balance between abstraction and user comprehension is always a trade-off. Showing explicitly what is required for each method does have advantages.

@Abyss-lord
Copy link
Contributor Author

It would be possible to extend PR #6343 to include full names in the method signatures. However, care must be taken to avoid making the implementation overly abstract. Striking the right balance between abstraction and user comprehension is always a trade-off. Showing explicitly what is required for each method does have advantages.

@justinmclean I’ve finished updating the code. Please take a look at the PR again when you have time, I think context is a great idea, and I want to extend PR #6343 when it merge.

@justinmclean
Copy link
Member

Let's wait for others to comment first before doing more work on the command context, as they may have some valuable input that saves rework.

@Abyss-lord
Copy link
Contributor Author

Let's wait for others to comment first before doing more work on the command context, as they may have some valuable input that saves rework.

@justinmclean sure, could you please review this PR when you have time? I just change the Cli output in this pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[EPIC] Make CLI more extendable and maintainable
4 participants
@Abyss-lord @tengqm @shaofengshi @justinmclean